Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

virtio_mem_hugetlb_migration: adds new test case #4126

Merged
merged 1 commit into from
Sep 20, 2024
Merged

virtio_mem_hugetlb_migration: adds new test case #4126

merged 1 commit into from
Sep 20, 2024

Conversation

mcasquer
Copy link
Contributor

@mcasquer mcasquer commented Aug 2, 2024

virtio_mem_hugetlb_migration: adds new test case

Creates a new test case for performing a live migration
of a VM with a virtio-mem device that consumes hugepages

Signed-off-by: mcasquer [email protected]
ID: 2653

@mcasquer mcasquer force-pushed the 2653_hugetlb_virtio_mem branch 2 times, most recently from 1cfde10 to 06b98b2 Compare August 2, 2024 09:19
@mcasquer mcasquer marked this pull request as ready for review August 2, 2024 09:31
@mcasquer
Copy link
Contributor Author

mcasquer commented Aug 2, 2024 

 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: STARTED
 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: PASS (113.00 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

@mcasquer
Copy link
Contributor Author

mcasquer commented Aug 2, 2024 •
edited
Loading

@zhenyzha @MiriamDeng @PaulYuuu @yanan-fu could you review this PR? Thanks !

@mcasquer
Copy link
Contributor Author

mcasquer commented Aug 2, 2024

@menli820 do you consider this test case could be executed in windows too?

@menli820
Copy link
Contributor

menli820 commented Aug 5, 2024

@menli820 do you consider this test case could be executed in windows too?

@mcasquer Thanks for the reminder.
Not currently as windows side not support migration well from the developer.

@zhenyzha
Copy link
Contributor

zhenyzha commented Aug 6, 2024

virtio_mem_hugetlb_migration.arm64-pci: PASS (116.79 s)

LGTM.Acked-by: Zhenyu Zhang [email protected]

@mcasquer
Copy link
Contributor Author

@PaulYuuu @yanan-fu this is a kindly reminder, please could you review this PR? Thanks !

PaulYuuu
PaulYuuu reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@PaulYuuu PaulYuuu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Others LGTM.

qemu/tests/cfg/virtio_mem_hugetlb_migration.cfg Outdated Show resolved Hide resolved
@mcasquer
Copy link
Contributor Author

@yanan-fu this is a kindly reminder, could you please review this PR? Thanks !

yanan-fu
yanan-fu reviewed Aug 19, 2024
View reviewed changes
qemu/tests/virtio_mem_hugetlb_migration.py Outdated
requested_size_vmem = params.get("requested-size_test_%s" % mem_object_id)
device_id = "virtio_mem-%s" % mem_object_id
node_id = params.get_numeric("node_memory_%s" % mem_object_id)
process.system("echo %d > /proc/sys/vm/nr_hugepages" % mem, shell=True)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With setup_hugepage=yes in cfg, env_process will setup hugepage based on some logic, checking hugepage size and setup the target hugepages.
Logically, i think it is conflict with the setting in the test case level.

And, nr_hugepages is the page number, i did not see any checking about the page size.

Copy link
Contributor Author

@mcasquer mcasquer Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanan-fu
I get that using nr_hugepages doesn't keep in mind the hugepages size, so likely that needs to be changed, but then how could be handled the self migraton of a VM using hugepages? AFAIU setup_hugepages only does it for the source VM, right? I need the double of those hugepages if we think on the destination too...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, setup_hugepage is about the action in host env, both src and dst vm can use the hugepage path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yanan-fu could you review again this PR? Thanks !

@yanan-fu
Copy link
Contributor

Compare this case with case virtio_mem_numa_basic.with_migration, The difference is use one virtio_mem instead of two and use memory-backend-file instead of memory-backend-rom, it can be controlled easily with configuration IMO.

and migrate with hugepage, add hugepage setup before launch vm.

@mcasquer
Copy link
Contributor Author

Compare this case with case virtio_mem_numa_basic.with_migration, The difference is use one virtio_mem instead of two and use memory-backend-file instead of memory-backend-rom, it can be controlled easily with configuration IMO.

and migrate with hugepage, add hugepage setup before launch vm.

@yanan-fu it's correct, I thought only using setup_hugepages with reserve=false and prealloc=true won't be sufficient

@mcasquer mcasquer force-pushed the 2653_hugetlb_virtio_mem branch 2 times, most recently from d54c1fc to 2c90c4b Compare August 26, 2024 07:20
@mcasquer
Copy link
Contributor Author 

 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: STARTED
 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: PASS (162.08 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

@mcasquer
Copy link
Contributor Author

This is a kindly reminder, @yanan-fu please could you review again this PR? Thanks !

@mcasquer
Copy link
Contributor Author

Hi again @yanan-fu this is a kindly reminder, please could you review this PR? Thanks !

@yanan-fu
Copy link
Contributor

Do we need to cover the negative scenario:

If the virtio-mem device is resized to a higher value, a warning message is displayed.
(qemu) qemu-kvm: warning: qemu_prealloc_mem: preallocating memory failed: Bad address

@mcasquer
Copy link
Contributor Author

mcasquer commented Sep 10, 2024 •
edited
Loading

Do we need to cover the negative scenario:

If the virtio-mem device is resized to a higher value, a warning message is displayed.
(qemu) qemu-kvm: warning: qemu_prealloc_mem: preallocating memory failed: Bad address

@yanan-fu How exactly do you reproduce that scenario? In this case if you resize the virtio-mem device to a higher value it should be:

(qemu) qom-set virtio_mem-vmem0 requested-size 9G
Error: 'requested-size' cannot exceed the memory backend size(0x200000000)

Which is the expected output (but it is not covered 😅 )

@yanan-fu
Copy link
Contributor

Do we need to cover the negative scenario:

If the virtio-mem device is resized to a higher value, a warning message is displayed.
(qemu) qemu-kvm: warning: qemu_prealloc_mem: preallocating memory failed: Bad address

@yanan-fu How exactly do you reproduce that scenario? In this case if you resize the virtio-mem device to a higher value it should be:

(qemu) qom-set virtio_mem-vmem0 requested-size 9G
Error: 'requested-size' cannot exceed the memory backend size(0x200000000)

Which is the expected output (but it is not covered 😅 )

It is not about reproduce the issue, i means do we need to cover this negative scenario in this test (as it is mentioned in the manual test case) ?

@mcasquer
Copy link
Contributor Author

Do we need to cover the negative scenario:

If the virtio-mem device is resized to a higher value, a warning message is displayed.
(qemu) qemu-kvm: warning: qemu_prealloc_mem: preallocating memory failed: Bad address

@yanan-fu How exactly do you reproduce that scenario? In this case if you resize the virtio-mem device to a higher value it should be:

(qemu) qom-set virtio_mem-vmem0 requested-size 9G
Error: 'requested-size' cannot exceed the memory backend size(0x200000000)

Which is the expected output (but it is not covered 😅 )

It is not about reproduce the issue, i means do we need to cover this negative scenario in this test (as it is mentioned in the manual test case) ?

@yanan-fu got it, yes I think it's interesting, I will send an update

@mcasquer
Copy link
Contributor Author

It is not about reproduce the issue, i means do we need to cover this negative scenario in this test (as it is mentioned in the manual test case) ?

@yanan-fu included !

@mcasquer
Copy link
Contributor Author 

 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: STARTED
 (1/1) Host_RHEL.m9.u5.ovmf.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.9.5.0.x86_64.io-github-autotest-qemu.virtio_mem_hugetlb_migration.q35: PASS (165.20 s)
RESULTS    : PASS 1 | ERROR 0 | FAIL 0 | SKIP 0 | WARN 0 | INTERRUPT 0 | CANCEL 0

yanan-fu
yanan-fu approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@yanan-fu yanan-fu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

@mcasquer
virtio_mem_hugetlb_migration: adds new test case
e9ca883 
Creates a new test case for performing a live migration
of a VM with a virtio-mem device that consumes hugepages

Signed-off-by: mcasquer <[email protected]>
@mcasquer
Copy link
Contributor Author

Hello, at this point could this PR be merged? Thanks!

@yanan-fu yanan-fu merged commit cf84766 into autotest:master Sep 20, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PaulYuuu PaulYuuu PaulYuuu left review comments

@yanan-fu yanan-fu yanan-fu approved these changes

@zhenyzha zhenyzha zhenyzha approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mcasquer @menli820 @zhenyzha @yanan-fu @PaulYuuu